Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ltex-ls language server #7838

Merged
merged 3 commits into from
Aug 23, 2023
Merged

Add ltex-ls language server #7838

merged 3 commits into from
Aug 23, 2023

Conversation

David-Else
Copy link
Contributor

ltex-ls now supports spelling and grammar correction for git-commit files, so I think it makes sense to support it by default. Everyone could benefit from it as Helix has no built-in spelling/grammar checker.

Screenshot from 2023-08-05 13-50-58

pascalkuthe
pascalkuthe previously approved these changes Aug 5, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this in my config. I think it makes sense to have this for git commits by default

@dead10ck
Copy link
Member

dead10ck commented Aug 5, 2023

Sorry, I disagree. I don't want an LS to spin up every time I do a git commit. Besides that, there are specific contexts where spell checking by default makes sense, but git is used for all kinds of technical projects where a spell checker would just be way too noisy. Even in the screenshot here there's a false positive on the LS's own name.

@David-Else
Copy link
Contributor Author

David-Else commented Aug 5, 2023

@dead10ck You are free to ignore the results, and there is no delay while the language server loads, so how come you don't want it spinning up in the background?

You can easily add words to the spellchecker in the config, even in multiple languages:

ltex.dictionary = { "en-US" = [
  "ltex-ls",
], "en-GB" = [
  "ltex-ls",
] } 

I also assume you can easily disable it in your config if you want.

@dead10ck
Copy link
Member

dead10ck commented Aug 5, 2023

@dead10ck You are free to ignore the results, and there is no delay while the language server loads, so how come you don't want it spinning up in the background?

It's wasteful to spin up a LS for editing a tiny document that's rarely open for more than a minute max, especially when it's unwanted in the first place.

You can easily add words to the spellchecker in the config, even in multiple languages:

ltex.dictionary = { "en-US" = [
  "ltex-ls",
], "en-GB" = [
  "ltex-ls",
] } 

Git is used for a wide variety of technical contexts using language that a spell checker will flag erroneously. Every time you use the name of a class, tool, abbreviation, variable, module, keyword, etc, etc, it will add noise to the document. They can't all go in an ignore list.

In my opinion, git is used in too many disparate contexts to justify turning on a spell checker by default. It's very visually distracting and difficult to ignore.

I also assume you can easily disable it in your config if you want.

This goes both ways. 🙂

Also, this is just my opinion. I don't have final say.

@clo4
Copy link
Contributor

clo4 commented Aug 8, 2023

Just my 2c as a user: I think @dead10ck is probably right, this shouldn't be a default, but it should be easy to enable — if there's a place to put "recipes" like enabling this (wiki or docs), that would be a great home for it!

@David-Else
Copy link
Contributor Author

People seem split on this, shall I just keep the general ltex-ls = { command = "ltex-ls" } and remove language-servers = [ "ltex-ls" ] from git-commit so it is slightly easier for people to make the change themselves?

Maybe @archseer needs to make an executive decision on this small but seemingly controversial change?

@the-mikedavis
Copy link
Member

I'm also hesitant to have this set for git-commits by default but adding a default config for ltex-ls so it's easy to reference in ~/.config/helix/languages.toml seems like a nice win to me

@pascalkuthe
Copy link
Member

yeah I agree, let's just add it so its easy to configure yourself, that's uncontroversial

Copy link
Member

@dead10ck dead10ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@David-Else
Copy link
Contributor Author

I have done as requested.

@David-Else David-Else changed the title Add ltex-ls as the default language server for git-commit files Add ltex-ls language server Aug 23, 2023
@the-mikedavis the-mikedavis merged commit c9694f6 into helix-editor:master Aug 23, 2023
6 checks passed
@David-Else David-Else deleted the ltex-ls branch August 23, 2023 19:20
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants